fix(security): create atomic-write temp files with restrictive permissions#531
fix(security): create atomic-write temp files with restrictive permissions#531qihan-bot wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
…sions Temp files created by atomic_write / atomic_write_async now use OpenOptions with mode 0o600 (owner-only read/write) instead of the default umask-derived permissions. This closes a brief window where sensitive credential and token data could be readable by other users before the rename completes. Also adds sync_all() before rename for crash-safety and a new test verifying the 0o600 permission on both sync and async paths.
🦋 Changeset detectedLatest commit: 07b5b11 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
googleworkspace-bot
added
the
area: core
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a security vulnerability by enhancing the atomic file writing mechanism. It ensures that temporary files, particularly those holding sensitive credentials or tokens, are created with highly restrictive permissions from the outset, preventing unauthorized access during the brief window before the atomic rename completes. Additionally, it improves data integrity by ensuring all data is written to disk before the final file swap. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances security by setting restrictive permissions (0o600) on temporary files used for atomic writes, preventing a race condition where sensitive data could be read by other users. The change is implemented for both synchronous and asynchronous atomic_write functions, includes a call to sync_all() for improved crash safety, and adds a test to verify the permissions on Unix systems. My review focuses on the platform-specific nature of the permission fix. While the changes are effective for Unix, they do not address the same vulnerability on other platforms like Windows. I've left comments suggesting a more cross-platform approach for a more complete security solution.
| #[cfg(unix)] | ||
| { | ||
| opts.mode(0o600); | ||
| } |
There was a problem hiding this comment.
This correctly applies restrictive permissions on Unix. However, this security fix is platform-specific and does not apply to Windows. On Windows, the temporary file for sensitive data will still be created with default permissions, which could allow other users on the system to read it. This leaves the security vulnerability open on Windows systems.
For a robust, cross-platform solution, I recommend using the tempfile crate (which is already a dev-dependency). It handles secure temporary file creation on major platforms by default, which would fully resolve this security issue.
| #[cfg(unix)] | ||
| { | ||
| opts.mode(0o600); | ||
| } |
There was a problem hiding this comment.
Similar to the synchronous version, this fix only applies restrictive permissions on Unix systems. The vulnerability of a readable temporary file remains on Windows.
To address this cross-platform, you could consider using a crate like async-tempfile, or refactor atomic_write to use the tempfile crate and then call it from this async function within a tokio::task::spawn_blocking block. This would ensure the temporary file is created securely on all supported platforms.
2 participants
Summary
fs::write/tokio::fs::writewithOpenOptions+mode(0o600)in bothatomic_writeandatomic_write_async, ensuring temp files containing credentials/tokens are only readable by the file owner beforerenamecompletessync_all()call beforerenamefor crash-safety (data hits disk before the atomic swap)test_atomic_write_permissionstest verifying0o600on both sync and async code pathsMotivation
atomic_writeis used bycredential_store,oauth_config, andtoken_storageto persist sensitive data (encrypted credentials, OAuth tokens). The previous implementation usedfs::writewhich creates files with umask-derived permissions (typically0o644), leaving a brief window where other users on the system could read the temp file beforerename.Test plan
cargo test fs_util— all 5 tests pass including new permission testcargo test— full suite 631 tests pass, no regressionscargo clippy -- -D warnings— clean